Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Usernames should encode all characters in emails #9541

Open
wants to merge 6 commits into
base: alpha
Choose a base branch
from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Jan 14, 2025

Pull Request

Issue

Closes: #9111

Approach

Tasks

  • Add tests

Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: usernames should encode all characters in emails fix: Usernames should encode all characters in emails Jan 14, 2025
Copy link

Thanks for opening this pull request!

@dblythy dblythy requested a review from a team January 21, 2025 10:52
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.49%. Comparing base (0a23023) to head (b6d7c71).

Files with missing lines Patch % Lines
src/Utils.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #9541      +/-   ##
==========================================
- Coverage   93.51%   93.49%   -0.03%     
==========================================
  Files         186      186              
  Lines       14807    14810       +3     
==========================================
- Hits        13847    13846       -1     
- Misses        960      964       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* @returns {String} The encoded string.
*/
static encode(input) {
return encodeURIComponent(input).replace(/[!'()*]/g, char =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here:

But GMail/GitHub/Facebook (etc) require escaping because people often put a closing parenthesis or square bracket or comma or period (etc) right after a URL in a sentence. In almost all cases, the URL didn't end with that punctuation, the punctuation was added by the user after pasting the URL. So the problem is that encodeURIComponent strictly follows the standard instead of dealing with certain non-conforming aspects of reality (that have good reasons to be non-conforming, imho).

How does it handle dot, comma, etc after the URL? Do we need to add these chars to the regex, or add tests for that?

emailAdapter: emailAdapter,
});

user.setUsername('hello :)');
Copy link
Member

@mtrezza mtrezza Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a loop and test a bunch of different chars? See also #9541 (comment).

@mtrezza
Copy link
Member

mtrezza commented Jan 21, 2025

@mathieulb does this address the issue you described?

@mtrezza
Copy link
Member

mtrezza commented Jan 21, 2025

@dblythy Does #8488 effectively make this PR obsolete? Or does it still make senes to add this improved escaping logic, since we can't predict what URLs we'll have in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants